-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Jingtan]iP #51
base: master
Are you sure you want to change the base?
[Jingtan]iP #51
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (nus-cs2113-AY1920S2#9)
Gradle defaults to an empty stdin which results in runtime exceptions when attempting to read from `System.in`. Let's add some sensible defaults for students who may still need to work with the standard input stream.
Add configuration for console applications
The OpenJFX plugin expects applications to be modular and bundled with jlink, resulting in fat jars that are not cross-platform. Let's manually include the required dependencies so that shadow can package them properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is pretty well-done, only suggestions were given to help with readability. Good job!
src/main/java/Duke.java
Outdated
@@ -1,10 +1,97 @@ | |||
import java.util.Scanner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For readability, it might be better to have a new line to separate main code and import lines.
src/main/java/Duke.java
Outdated
userResponde = in.nextLine(); | ||
dukeResponde(userResponde); | ||
|
||
}while(!userResponde.equals("bye")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if there was a space between } and "while" so that it is easier to read.
src/main/java/Duke.java
Outdated
if(userResponde.equals("bye")){ | ||
sayBye(); | ||
} | ||
else if(userResponde.equals("list")) { | ||
listTask(); | ||
} | ||
else if(userResponde.startsWith("done")) { | ||
markAsDone(userResponde); | ||
} | ||
else{ | ||
addNewTask(userResponde); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With reference to the coding standard, it is recommended that "else" is on the same line as "}" of the previous case.
if (xxx) {
} else {
}
src/main/java/Duke.java
Outdated
if(userResponde.startsWith("todo")){ | ||
tasks[taskCount] = new Todo(userResponde.substring(5)); | ||
} | ||
else if(userResponde.startsWith("deadline")){ | ||
int dividerPosition = userResponde.indexOf(" /by"); | ||
String taskName = userResponde.substring(9,dividerPosition); | ||
String deadlineTime = userResponde.substring(dividerPosition + 5); | ||
tasks[taskCount] = new Deadline(taskName,deadlineTime); | ||
} | ||
else if(userResponde.startsWith("event")){ | ||
int dividerPosition = userResponde.indexOf(" /at"); | ||
String taskName = userResponde.substring(6,dividerPosition); | ||
String deadlineTime = userResponde.substring(dividerPosition + 5); | ||
tasks[taskCount] = new Event(taskName,deadlineTime); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With reference to the coding standard, it is recommended that "else" is on the same line as "}" of the previous case.
if (xxx) {
} else {
}
src/main/java/Duke.java
Outdated
} | ||
|
||
public static void markAsDone(String userResponde){ | ||
int doneCount = Integer.parseInt(userResponde.substring(5)) - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name for this seems to be slightly misleading as count is usually used to keep count, this is more of an index. Just an opinion though.
src/main/java/Task.java
Outdated
return (isDone ? "\u2713" : "\u2718"); //return tick or X symbols | ||
} | ||
|
||
public String getDescription(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, it might be better to decide on having a space between "()" and "{"
src/main/java/Duke.java
Outdated
public class Duke { | ||
|
||
private static Task[] tasks = new Task[100]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was mentioned not to use magic numbers, perhaps a static int could be used instead
… on a specific date.
Add Branch level 8
Add Branch level 9
Add a java doc
No description provided.